Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lint: Black -> Ruff formatter #14896

Merged
merged 2 commits into from
May 2, 2024
Merged

Lint: Black -> Ruff formatter #14896

merged 2 commits into from
May 2, 2024

Conversation

ogajduse
Copy link
Member

@ogajduse ogajduse commented Apr 26, 2024 •
edited
Loading

Problem Statement

#14781 attempts to upgrade some of our pre-commit hooks including black. The new version of black formats 150+ files. Given the patch size, I propose switching to Ruff formatter as the patch size is comparable to the patch for the new version of black.

Reason to switch to Ruff

(copy&paste from one of the comments in this PR)
Astral's page sums it up pretty well. astral.sh/blog/the-ruff-formatter

I would like us to switch to Ruff mainly because of its speed compared to black. It is incredibly fast and the compatibility with Black is 99%. Looking at the patch, it might not look like 99%, but taking into account how big our project is and what changes were made, it is not that bad.

Solution

  • Remove the black formatter and substitute it with Ruff formatter,
  • bump the version of ruff pre-commit hook.

Related Issues

Related PR: #14781

@ogajduse ogajduse added CherryPick PR needs CherryPick to previous branches 6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels Apr 26, 2024
@ogajduse ogajduse self-assigned this Apr 26, 2024
@ogajduse ogajduse requested review from a team as code owners April 26, 2024 11:54
@ogajduse ogajduse force-pushed the lint/black-to-ruff-formatter-n-upgrade branch from 8dc333d to bed6d60 Compare April 26, 2024 13:01
synkd
synkd reviewed Apr 26, 2024
View reviewed changes
Copy link
Contributor

@synkd synkd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small, non-blocking suggestions, but looks good to me overall.

tests/foreman/api/test_errata.py Outdated Show resolved Hide resolved
tests/foreman/api/test_errata.py Outdated Show resolved Hide resolved
JacobCallahan
JacobCallahan approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Merge pending Danny's recommendations

lhellebr
lhellebr approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@lhellebr lhellebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK but is there some specific reason for us to prefer Ruff?

@ogajduse
Copy link
Member Author

I think this is OK but is there some specific reason for us to prefer Ruff?

Astral's page sums it up pretty well. https://astral.sh/blog/the-ruff-formatter

I would like us to switch to Ruff mainly because of its speed compared to black. It is incredibly fast and the compatibility with Black is 99%. Looking at the patch, it might not look like 99%, but taking into account how big our project is and what changes were made, it is not that bad.

@jyejare
Copy link
Member

jyejare commented Apr 30, 2024

@ogajduse Could we run some tests before merge it ? Especially the code where reviewers commented/discussed concerns ?

@ogajduse
Copy link
Member Author

@ogajduse Could we run some tests before merge it ? Especially the code where reviewers commented/discussed concerns ?

Sure we can! I do not really see a need to do so, but @Gauravtalreja1 and/or @shweta83, feel free to trigger PRT here.

@jyejare
Copy link
Member

jyejare commented May 1, 2024

Sure we can! I do not really see a need to do so

AFAIR, In history, we have broken the sanity when we merged a large formatting PR, So I see the need here.

@ogajduse ogajduse force-pushed the lint/black-to-ruff-formatter-n-upgrade branch from 9d648a7 to 6f771d8 Compare May 2, 2024 09:37
@ogajduse
Copy link
Member Author

ogajduse commented May 2, 2024

trigger: test-robottelo
pytest: tests/foreman/api/test_notifications.py tests/foreman/ui/test_ansible.py::TestAnsibleCfgMgmt::test_positive_assign_ansible_role_variable_on_host -k "long_running_task or test_positive_assign_ansible_role_variable_on_host"

@Gauravtalreja1 Gauravtalreja1 added the AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing label May 2, 2024
@jyejare jyejare merged commit 3f11c65 into SatelliteQE:master May 2, 2024
10 checks passed
This was referenced May 2, 2024
Closed
Closed
Closed
Closed
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
7ec8dca 
* Lint: Black -> Ruff formatter

* Address review comments
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
08e2475
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
d66b5a5
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
d862bb0
This was referenced May 3, 2024
Merged
Merged
Merged
Merged
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
@ogajduse
[6.14.z] Lint: Black -> Ruff formatter (#14896) (#14945)
3364bb8 
Lint: Black -> Ruff formatter (#14896)
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
@ogajduse
[6.12.z] Lint: Black -> Ruff formatter (#14896) (#14943)
11b9d89 
Lint: Black -> Ruff formatter (#14896)

* Lint: Black -> Ruff formatter

* Address review comments
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
d735b07
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
@ogajduse
Lint: Black -> Ruff formatter (SatelliteQE#14896)
6a4519a
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
@ogajduse
[6.15.z] Lint: Black -> Ruff formatter (#14896) (#14946)
c6e9352 
Lint: Black -> Ruff formatter (#14896)
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
@ogajduse
[6.13.z] Lint: Black -> Ruff formatter (#14896) (#14944)
7f9ab57 
Lint: Black -> Ruff formatter (#14896)
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
@ogajduse @jyejare
Lint: Black -> Ruff formatter (SatelliteQE#14896)
db4edf4 
* Lint: Black -> Ruff formatter

* Address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@synkd synkd synkd left review comments

@shweta83 shweta83 shweta83 left review comments

@Gauravtalreja1 Gauravtalreja1 Gauravtalreja1 approved these changes

@lhellebr lhellebr lhellebr approved these changes

@JacobCallahan JacobCallahan JacobCallahan approved these changes

@jyejare jyejare Awaiting requested review from jyejare

Assignees

@ogajduse ogajduse

Labels
6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ogajduse @jyejare @JacobCallahan @lhellebr @Gauravtalreja1 @shweta83 @synkd